Use exact distinct_count from statistics if exists for COUNT(DISTINCT column)) calculations#20845
Conversation
github-actions
bot
added
core
| // expressions like casts or literals are not supported. | ||
| let col_expr = expr.as_any().downcast_ref::<expressions::Column>()?; | ||
| if let Precision::Exact(dc) = col_stats[col_expr.index()].distinct_count { | ||
| return Some(ScalarValue::Int64(Some(dc as i64))); |
There was a problem hiding this comment.
This assumes NULL is not counted in distinct_count column. AFAIS from the repository it is not counted so this should be ok but wanted to put a comment here
jonathanc-n
left a comment
jonathanc-n
left a comment
There was a problem hiding this comment.
Thanks @buraksenn couple comments
| Ok(()) | ||
| } | ||
|
|
||
| fn mock_data_with_distinct_count( |
There was a problem hiding this comment.
Are we able to combine these two functions
There was a problem hiding this comment.
I did not exactly understand but changed it to one table driven test
| // expressions like casts or literals are not supported. | ||
| let col_expr = expr.as_any().downcast_ref::<expressions::Column>()?; | ||
| if let Precision::Exact(dc) = col_stats[col_expr.index()].distinct_count { | ||
| return Some(ScalarValue::Int64(Some(dc as i64))); |
There was a problem hiding this comment.
Distinct count is usize, we should deal with the case where it could overflow
There was a problem hiding this comment.
Since other paths were casting as well I did not do it actually but let me do it for all paths
|
Thanks @xudong963 @buraksenn and @jonathanc-n |
4 participants
Which issue does this PR close?
Does not close but part of #20766
Rationale for this change
If
distinct_countis known for the column from statistics we can directly use it inCOUNT(DISTINCT column))expressions instead of calculating.What changes are included in this PR?
Are these changes tested?
Added additional tests
Are there any user-facing changes?
No